Allow updates to all item slots #737
Conversation
hackaugusto
changed the title
| }, | ||
| }, | ||
| SlotItem::new_value(0, 0, [ONE, ONE, ONE, ONE]), | ||
| SlotItem::new_value(2, 0, [ONE, ONE, ONE, ZERO]), |
There was a problem hiding this comment.
I'm not super found of 0, 0, since it is a bit obscure what these values mean without IDE support. But adding wrappers types on this PR would be a lot more changes, so for now this is mostly to make the code more compact, and to make the arity explicit.
There was a problem hiding this comment.
I think the need for index will go away as a part of #667 refactoring. Specifically, if we represent storage commitment not as a Merkle tree of depth 8 but as a sequential hash of storage slots, the need for SlotItem struct will go away and AccountStorage may end up looking like this:
pub struct AccountStorage {
slots: Vec<StorageSlot>,
commitment: Digest,
}
pub enum StorageSlot {
/// Value of arity 0
SimpleValue(Word),
/// Value of arity > 0
ComplexValue(Vec<Word>),
Array(StorageArray),
Map(StorageMap),
}
| match self { | ||
| StorageSlotType::Value { .. } => SimpleSmt::<STORAGE_TREE_DEPTH>::EMPTY_VALUE, | ||
| StorageSlotType::Map { .. } => EMPTY_STORAGE_MAP, | ||
| StorageSlotType::Array { .. } => EMPTY_WORD, |
There was a problem hiding this comment.
I'm not sure if EMPTY_WORD is the best value to represent an empty array of any arity. I assumed the value would be the commitment to the array contents.
There was a problem hiding this comment.
Maybe all of these should be SimpleSmt::<STORAGE_TREE_DEPTH>::EMPTY_VALUE? Now that I think of it, we don't insert SimpleSmt::<STORAGE_TREE_DEPTH>::EMPTY_VALUE in the account storage constructor.
Edit: Ah, nvm, we do, because the default type is value, to change it we have to initialize the storage with an item that would set the initial value.
* fix: applying account delta for storage maps * rebasing on top * changing to BTreeMap * making tests pass * after review
Dominik1999
force-pushed
the
hacka-allow-updates-to-all-item-slots
branch
from
0a2e002 to
dfc0250
Compare
Dominik1999
force-pushed
the
hacka-allow-updates-to-all-item-slots
branch
from
dfc0250 to
c5e5fe7
Compare
Dominik1999
force-pushed
the
hacka-allow-updates-to-all-item-slots
branch
from
7e163ef to
33a16f4
Compare
There was a problem hiding this comment.
Looks good! Thank you! As discussed in #737 (comment), I'll merge this now but we need to follow this up with a PR which fixes how account storage delta is put together and applied.
Specifically:
AccountStorageDelta.cleared_itemsandAccountStorageDelta.updated_itemsshould contain updates only to value slots.AccountStorageDelta.updated_mapsshould contain updates to storage maps.
This should be sufficient to apply the delta because new roots of storage maps can be computed from the data in updated_maps and can then use these roots to update the corresponding storage slots.
To make the above work, we may need to modify MASM - but there may also be a way around it. For example, if the a set_item event is fired on a map storage slot, transaction host can verify that it is correct, but then ignore it.
Separately, we should also implement AccountStorage::set_map_item() and AccountStorage::get_map_item() methods to mimic the ones in the transaction kernel.
| self.set_map_item(key, EMPTY_WORD)?; | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Not for this PR - but this should probably return the new root of the storage map.
| // this will also update roots of updated storage maps, and thus should be run after we | ||
| // update storage maps - otherwise the roots won't match | ||
|
|
||
| for &slot_idx in delta.cleared_items.iter() { |
There was a problem hiding this comment.
I've also added this code which ensures that slots corresponding to storage maps can be set only to the roots of these maps. So, if we try to update the roots first (before updating the maps themselves), the assertion would get triggered.
| @@ -283,6 +279,18 @@ impl AccountStorage { | |||
| storage_map.apply_delta(map_delta)?; | |||
| } | |||
|
|
|||
| // --- update storage slots ------------------------------------------- | |||
| // this will also update roots of updated storage maps, and thus should be run after we | |||
| // update storage maps - otherwise the roots won't match | |||
There was a problem hiding this comment.
I think the order doesn't matter, the storage root update is queued in updated_items and the maps update in updated_maps. Both are triggered in MASM in set_map_item
There was a problem hiding this comment.
In combination with the code I linked above, the order does matter (otherwise tests won't pass). But also, this code is separate from MASM: when executing MASM we are building the delta, here we are applying it - which happens later.
Bugfix for set_item with a mapping.
This PR also: